Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Go: Add helper predicate lookThroughPointerType #17792

Merged
merged 3 commits into from
Oct 18, 2024
Merged

Go: Add helper predicate lookThroughPointerType #17792

merged 3 commits into from
Oct 18, 2024

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Oct 17, 2024 •
edited
Loading

I got fed up of the extra boilerplate code needed to do this in various different situations. Performance-wise I'm not sure if it's better to mark this pragma[inline] or if it would be better to leave it as it is. (Or even pragma[noinline]!) I will make sure to fix any performance problems before merging.

I'm open to suggestions for a better predicate name.

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Oct 17, 2024
@owen-mc owen-mc requested a review from a team as a code owner October 17, 2024 10:29
@github-actions github-actions bot added the Go label Oct 17, 2024
smowton
smowton reviewed Oct 17, 2024
View reviewed changes
go/ql/lib/semmle/go/Types.qll Outdated
@@ -489,8 +485,7 @@ class StructType extends @structtype, CompositeType {
*/
private predicate hasEmbeddedField(Type tp, int depth) {
exists(Field f | this.hasFieldCand(_, f, depth, true) |
tp = f.getType() or
tp = f.getType().(PointerType).getBaseType()
tp = lookThroughPointerType(f.getType())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this one may change behaviour, since tp would have had multiple values before (unlike all of the others, where the type of tp is disjoint with PointerType)

Copy link
Contributor Author

@owen-mc owen-mc Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, that's a mistake. I consciously skipped some other cases like that. I'll revert this one.

@owen-mc
Copy link
Contributor Author

owen-mc commented Oct 17, 2024

Here is the RA for it on a random db (allegro/akubra):

[2024-10-17 22:16:19] Evaluated non-recursive predicate Types::lookThroughPointerType/1#ccd91091@80ca5634 in 0ms (size: 19882).
Evaluated relational algebra for predicate Types::lookThroughPointerType/1#ccd91091@80ca5634 with tuple counts:
        19882   ~4%    {1} r1 = SCAN types OUTPUT In.0
                       {1}    | AND NOT _types_10#join_rhs#antijoin_rhs#3(FIRST 1)
        17436   ~6%    {2}    | SCAN OUTPUT In.0, In.0
                   
         2446   ~3%    {2} r2 = JOIN base_type WITH types ON FIRST 1 OUTPUT Lhs.0, Lhs.1
                   
        19882   ~3%    {2} r3 = r1 UNION r2
                       return r3

The performance impact seems absolutely miniscule. I think this is ready to merge. I'm happy to do another DCA or even QA if there is any chance that it might affect performance or alerts.

@owen-mc owen-mc merged commit b0376d5 into github:main Oct 18, 2024
14 checks passed
@owen-mc owen-mc deleted the go/lookthrough-pointer-type branch October 18, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smowton smowton smowton approved these changes

Assignees
No one assigned
Labels
Go no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@owen-mc @smowton